-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ref(parser): limit max stack lines we parse #7410
Conversation
size-limit report 📦
|
packages/utils/src/stacktrace.ts
Outdated
@@ -79,7 +86,7 @@ export function stripSentryFramesAndReverse(stack: StackFrame[]): StackFrame[] { | |||
|
|||
// The frame where the crash happened, should be the last entry in the array | |||
return localStack | |||
.slice(0, STACKTRACE_LIMIT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AbhiPrasad @timfish am I right to assume that the slice should no longer be required as everything will run through the stack parser on L19? Not sure if we can remove it, or if there is some other code path that could call reverseSentryFrames fn. I kept it as the fn is exported, so better be safe than sorry here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, we can probably remove the slice
, but perhaps someone depends on this behaviour.
We could just mark it to be removed in the next major version with a TODO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use stripSentryFramesAndReverse
in the Electron SDK so we can combine two parsers:
https://github.com/getsentry/sentry-electron/blob/0d2eb16bf8daf9e330da85a9a823d20c80ad16ac/src/renderer/stack-parse.ts
However as long as I remember when updating, I can update the above code to drop out of the for loop at STACKTRACE_LIMIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A further search across GitHub suggests this is only used in the Electron SDK. I've opened an issue to track this so feel free to remove the slice here and I'll fix this downstream on the next javascript sdks update!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am somewhat still not super happy with removing it entirely - it is relatively low cost and provides safety. The risk/reward does not seem worth it to me. If we are to remove it then I think we should do it in the next major just in case someone out there relies on this behavior that our search did not show missed.
I would however propose we make the slice conditional and only do it if the input exceeds the max array size, that seems like a good middle ground without breaking anything.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, my point is moot because the input is a ReadonlyArray<StackFrame>
and since our implementation is reversing the array, a copy is required regardless so that we do not end up mutating the input.
I made the changes to use readonly type in #7417 as it seemed reasonable and safer (the implementation was already considering it as readonly).
It becomes a larger change in terms of how we expect the function to be called and what safety guarantees we want, e.g. if we only expect it to be called once (which seems to be the case), then we might not care about readonly type.
For now, I would opt to keep this as is for now and port the for loop changes to electron - this way we just avoid parsing data that will be discarded anyways and keep some extra safety in stripSentryFramesAndReverse
57c3c64
to
fb1abb4
Compare
Enforce the stacktrace frame limit when we parse frames